Batch: pre-claim inline steps in the same batch - #3568

AI [question]: This disjunct may contradict the reasoning behind the lone-inline exclusion, because the claims it replaces were already concurrent.

runtime.ts invokes run() inside inlineExecutions.map(...), so N lazy step_started POSTs go out in parallel — N concurrent claims cost ~1 RTT, not N. The exclusion just above is justified as "a pair-only batch costs the same round trip as the single claim while giving up the claim/body overlap and bump-and-report", and that argument generalizes past N=1: a pair-only batch of any size also costs one round trip and also gives up the overlap.

It is also the common shape rather than an edge case. With MAX_INLINE_STEPS = 3 (constants.ts:167), a plain 3-step Promise.all fan-out is 3 inline + 0 eager: the second disjunct evaluates 3 - 3 + 0 = 0, but size >= 2 is true, so it folds — trading turbo's claim/body overlap for no round-trip saving.

There is a good counter-argument the description doesn't make: one POST is a single latency sample where N concurrent claims are a max-of-N, and the trace's own 356/1110/375 spread shows the tail dominates. Folding may well win on p99 for that reason alone. But if that is the justification it should be the stated one, since the round-trip argument doesn't survive the claims being concurrent.

Has the N=2..3-with-nothing-else case been measured? If the tail argument holds, worth recording it in this comment; if not, the gate arguably wants the "≥1 other batchable event" disjunct only.

Answering rather than changing the gate, because the data to justify a change isn't there.

You're right that the round-trip argument doesn't survive the claims being concurrent. runtime.ts maps inlineExecutions and launches all of them, so N lazy claims are ~1 RTT wall-clock, not N. A pair-only batch of 2–3 saves no round trip and does give up the optimistic claim/body overlap. The description's reasoning generalises past N=1 in the wrong direction.

The tail argument you offer is the plausible one — one POST is a single sample where N concurrent claims are a max-of-N, and the trace's 356/1110/375 spread says the tail dominates. But it is unmeasured. The benchmark has no N=2..3 scenario: fanout-ttfs is Promise.all(100 steps), which always folds and always has eager company, so it cannot separate the pair-only case. Nothing on this PR tells us whether a 3-step Promise.all got faster or slower.

That matters more than it looks, because the benchmark shows a TTFS regression on the three scenarios that structurally cannot fold at all (posted separately). Until that is explained I would not touch this gate in either direction — narrowing it to the "≥1 other batchable event" disjunct is a behaviour change we would also be shipping unmeasured.

Concretely: this wants a Promise.all(3 steps) benchmark scenario before the ≥2 disjunct is either justified or removed. Filing that as the follow-up rather than guessing here.

Correcting my earlier reply on this thread: I framed the trade-off as if optimistic inline start were always the baseline. It is not, and that changes the answer depending on configuration.

isOptimisticInlineStartEnabled() is false when unset. Turbo is on by default and passes forceOptimisticStart, but only under a narrow condition (runtime.ts):

const forceOptimisticStart =
  turbo &&
  !suspensionResult.hasAttributeEvents &&
  !suspensionResult.waitTimeout &&
  !suspensionResult.hasHookEvents &&
  !suspensionResult.hasAwaitedHookCreation &&
  !openHookWaitState.openHook &&
  !openHookWaitState.openWait;

So there are two regimes, and your critique lands differently in each:

Optimistic start active (turbo, no attr/wait/hook events, no open hook or wait). The body runs before the claim is confirmed. Folding into a pair gives up that overlap, and — as you argued — saves no round trip, because the N lazy claims already went out concurrently. This is the case where the >= 2 disjunct has a real cost and only the tail argument (one sample vs max-of-N) could justify it. Still unmeasured.

Optimistic start inactive (a wait timeout, an open hook or wait, WORKFLOW_TURBO=0, an explicit WORKFLOW_OPTIMISTIC_INLINE_START=0, or suppressOptimisticStart). The lazy path already awaits the claim before running the body, so there is no overlap to give up. Folding is close to free here, and the durable-running-before-body window the pairs introduce already exists on this path.

Worth noting the overlap with the fold's own gate is only partial: batchFanoutEligible excludes hook and attribute writes, so those never reach either mechanism — but waitTimeout and openHookWaitState are not in the fold's gate. A fan-out alongside a pending wait or an open hook therefore folds into pairs while optimistic start is off, which is exactly the regime where the fold costs nothing in overlap terms.

So the honest position on the gate: your objection is correct in the optimistic regime and does not apply in the awaited one. A Promise.all(3 steps) benchmark scenario would still settle it, and it should be run in both regimes rather than one.

AI [question]: This opt-in changes an ordering property that held before it, and I'd like to confirm nothing downstream depends on the old one.

Bodies start off the pair chunk's commit while trailing chunks ride deferredBatchWork, so a fast inline body can write step_completed before a trailing chunk commits its step_createds. Previously — including #3025 — await Promise.all(dispatches) gated the bodies, so every create in the fold was durable before any body ran. The new contract is only "every create durable before ack", which is strictly weaker: the log can now hold a step's terminal event at a lower slot than a sibling's created event.

The replay path looks safe: matching is by correlation id, slots stay dense, and creates are idempotent, so a redelivery after a trailing-chunk failure re-creates the missing steps correctly.

What I can't rule out is consumers outside the replay path — the ClickHouse analytics ingest and the run-details UI reconstruct run shape from the event stream, and either could reasonably assume created-precedes-terminal globally rather than per-step. Is that assumption made anywhere? Asking for confirmation rather than a change.

Checked both consumers; neither assumes it.

front trace viewer (packages/web-shared/src/lib/trace-builder.ts): groupEventsByCorrelation buckets events by correlationId, so the only ordering it consumes is per-step — which the pair preserves. cascadeSpans sorts by startTime (timestamps), not by slot or array position, and computeLatestKnownTime takes a max. Nothing reads log order.

ClickHouse (workflow_observability.workflow_runs_v1, fed by mv_workflow_runs_v1 from workflow_operations_raw_v2): every column is a commutative aggregate — sum/min/max/groupUniqArrayArray — and every argMax keys on a source timestamp, not on slot. latestRunEvent is argMaxIf by timestamp over run-level events only. workflow_operations_raw_v2 itself is one row per event with no ordering semantics. Usage facts are emitted per committed event server-side, so they inherit nothing either.

So the weaker contract holds for both. The one thing worth knowing for anyone reading those tables: a batch stamps one occurredAt for the whole chunk, so sibling events in a fold can share a timestamp and argMax ties resolve arbitrarily among them. That predates this PR (it came in with the fold in #3025) and no current column depends on breaking such a tie.

AI [suggested fix]: completed looks like the wrong value here, and reusing it costs the attribute its only useful distinction.

StepSkipReason is typed Step['status'] (semantic-conventions.ts:327). The pre-existing site further down this file sets completed on the EntityConflictError path whose comment reads "Step in terminal state, skipping" — accurate there. But this site's own comment says "a concurrent writer owns this step", which is most likely running, and the pair's 409 doesn't actually reveal the winner's status at all.

Those two are the only places StepSkipped / StepSkipReason are set anywhere in the package, so tagging both completed means the attribute reads 100% completed and cannot separate "skipped because already done" from "skipped because it lost the claim" — which is the question you'd query it for.

Suggest running, or omitting the reason since the verdict doesn't carry one.

You're right, and my own review pass got this wrong — I checked it against the other call site, concluded "consistent with the lazy path", and stopped. Consistency was the wrong test: both sites emitting completed is exactly what makes the attribute useless.

Changed to running in 53e933a. Took that over omitting so the two skip classes stay distinguishable in a query rather than one of them becoming an absent attribute. Comment notes that the 409 only proves the step exists and that its claim winner is the one executing.

AI [note]: This fix is partial under the round-2 architecture, and the description reads as unconditional.

batchCommittedSlotCeiling only folds in slots from chunks that have committed, but the bodies start off the pair chunk while trailing chunks are still in flight. So on a multi-chunk fold, an inline terminal write issued before the trailing chunks land still names a position below them and still draws a skipped-slot report — the thing this change removes, partially reintroduced by the per-chunk deferral.

Bounded (trailing chunks only, big fan-outs only) and self-correcting, so not worth restructuring. Worth narrowing the claim to single-chunk folds so the next reader doesn't chase a report that is expected.

Fixed in 53e933a — narrowed in the batchCommittedSlotCeiling docstring: the echo is only fully suppressed for a single-chunk fold, and on a multi-chunk fan-out an inline terminal write issued before the trailing chunks land still names a position below them and still draws a report. Bounded and self-correcting, recorded so it reads as expected rather than as a bug.

AI [suggested fix]: The shape string isn't canonical. It's built from a Map in first-seen order, so identical batch compositions emit step_created:17,step_started:3 or step_started:3,step_created:17 depending on frame order — and pre-claimed pairs change that order relative to a pure eager fold.

Sorting the entries before joining makes this a groupable dimension instead of a string every consumer has to parse and re-normalize.

Fixed in 53e933a — entries sorted by event type before joining, with a comment recording why (Map iteration is first-seen order, so a pre-claimed fold and a pure eager fold rendered the same composition as different strings).

AI [note]: Worth recording where this throw lands: the pair is already durable by this point, so the failure mode is "step claimed, body never runs, recovered on redelivery via owned-recovery" rather than "request fails cleanly". Fine for a defensive assert on an unreachable path — just worth a clause in the comment, since "unreachable" here still costs a redelivery rather than being free.

Added in 53e933a — the comment now says the pair is already durable at that point, so the throw lands with the step claimed and its body unrun, recovered on redelivery through owned-recovery rather than the request failing cleanly.

AI Review: Blocking

A pair-chunk rejection escapes the handler while the trailing chunks are still in flight, which is the thing settlePhase exists to prevent.

On this path the foreground awaits only commits[pairChunkIndex]. When that rejects, the flush op rejects, settlePhase(ops) sees the failure, and handleSuspension throws — but trailing (the sibling chunk commits and every chunk's publishes) is only .catch(() => {})'d, never joined. deferredBatchWork is never handed to the caller either, since suspensionResult was never assigned, so nothing downstream can join it.

settlePhase's own docstring states why this matters: "a sibling create that lands after the rejection escaped commits an event whose correlation id came from the abandoned replay's seeded sequence … and it races the restart's reload while doing so." The runtime's catch (suspensionError) re-derives the replay in-process, so the restart runs concurrently with those writes.

This is new in the second commit. With sequential chunks a failed chunk meant later chunks never POSTed at all.

Verified locally (34 steps, one pair, gated createBatch, chunk 1 = pair chunk rejects with a 500 while chunk 2's POST is outstanding):

✓ escapes with the trailing chunk still in flight
[probe] published 3 step messages AFTER handleSuspension rejected

expect(trailingSettled).toBe(false) holds at the moment the handler rejects, and chunk 2 then commits its creates and publishes its 3 queue messages afterwards. The non-opt-in path (await trailing) is fine; only the opt-in path has the hole, and the runtime always opts in.

Settling before rethrowing looks sufficient:

if (pairChunkIndex >= 0) {
  try {
    await commits[pairChunkIndex];
  } catch (err) {
    await trailing.catch(() => {});
    throw err;
  }
}

Same class of gap on the caller side: anything that throws in runtime.ts between the handler's return and the Promise.all([dispatchesSettled, deferredBatchWork]) join (assertNoInFlightOwnedSteps, computeStepLatencyTracking, the executor setup) abandons the deferred work too. Worth a comment at least about why that one is acceptable.

Fixed in 1c64070. The allowDeferredBatchWork failure path now settles trailing before rethrowing:

try {
  await Promise.all(pairCommits);
} catch (err) {
  await trailing.catch(() => {});
  throw err;
}

Regression test added (settles the trailing chunk before a pair-chunk failure escapes) — it asserts probe(pending) === 'pending' and trailingSettled === false at the moment the pair chunk rejects, then that both flip once chunk 2 is released. Verified it fails on the pre-fix code (probe returns settled).

Left the caller-side gap alone: anything throwing in runtime.ts between the handler's return and the Promise.all([dispatchesSettled, deferredBatchWork]) join still abandons the deferred work. That one bottoms out in delivery failure plus redelivery, so it converges without a new barrier — flagging it here rather than changing it.

AI Review: Blocking

A transport retry of a pair chunk that already committed converges to 409 on both rows, and this branch reads that as "a concurrent writer owns the step" — so the invocation that actually owns it skips the body.

world-vercel's createBatch computes retryConvergent = true for a [step_created, step_started] pair (from #3025) and passes batchIdempotent: true to withEventPostRetry. So on UND_ERR_REQ_RETRY / a socket timeout / a transient 5xx after the transaction committed, the retry returns HTTP 200 with per-item 409s, and nothing in the response distinguishes "someone else created it" from "my own first attempt created it".

Result: owned: false → executeStep returns skipped before any write. The step is running, stamped with this message's ownerMessageId, and nobody is executing it. The caller does not dispatch it either (it is in inlineCorrelationIds). Recovery waits for the ownership lease: isStepOwnershipActive is true and step.ownerMessageId !== metadata.messageId on every later wake, so the dispatch pass arms a delayed backstop for stepLeaseRemainingSeconds — up to INLINE_OWNERSHIP_LEASE_SECONDS = 860.

The lazy path this replaces does not have that failure mode, deliberately: EVENT_RETRY_ELIGIBILITY.step_started.retryable === false, so a lost response fails the delivery, the message redelivers in seconds, and owned-recovery re-executes. The pair path converts that into a silent ~14-minute stall for the step.

Two directions, either works:

• drop step_started-carrying batches from retryConvergent (accept the redelivery, matching the single path's reasoning), or
• keep the retry but make a pair 409 on a retried batch fail the delivery instead of resolving to owned: false — withEventPostRetry would need to report that it re-attempted.

The owned: false reading is only sound for a first attempt.

Fixed in 1c64070, taking the first of the two options: retryConvergent in world-vercel now excludes any batch carrying a step_started, pair or not.

const retryConvergent = events.every(
  ({ event }) =>
    event.eventType !== 'step_started' && event.eventType !== 'step_retrying'
);

Trade-off worth naming: a transient blip on the pair chunk now fails the delivery even when nothing landed, where the retry would sometimes have absorbed it. That is exactly the single-POST path's existing trade-off for step_started, so it is no worse than the lazy path it replaces — but if the lost retry turns out to matter for a wide fan-out's first chunk, the follow-up is to have withEventPostRetry report that it re-attempted and fail only the ambiguous case.

The existing retries a transient 5xx in-process test used transitionEvents(), which contains a born-running pair, so it was retargeted at a creates-plus-terminal batch and a new test pins the pair case as single-attempt. Spec docstring in packages/world/src/events.ts updated too: the pair does converge, it is just not actionable for the caller, which is the part the old wording missed.

AI Review: Note

findIndex assumes at most one chunk carries pairs, and nothing enforces the constant coupling that makes that true.

Today it holds: lazyInlineCorrelationIds is the first getMaxInlineSteps() entries of stepItems, so pairs always occupy orders 0..2N-1 and sort to the front, and 2 * MAX_MAX_INLINE_STEPS === 32 === MAX_BATCH_FANOUT_EVENTS. So every pair lands in chunks[0].

If either constant moves — MAX_MAX_INLINE_STEPS up, or MAX_BATCH_FANOUT_EVENTS down to track a smaller server budget — pairs spill into chunks[1]. The chunker keeps each pair intact (that part is fine), but pairChunkIndex only gates on the first pair-carrying chunk, so a spilled pair returns no inlineClaims entry. runtime.ts then falls back to lazyStepInput for that step and executeStep sends a lazy step_started — racing this same invocation's still-in-flight deferred pair for the same step. Whichever loses gets a 409; if the lazy start loses, executeStep returns skipped, nobody runs the body, and it becomes the 860s-lease stall described in the other comment.

The PR description says "the chunker refuses anyway should the constants diverge" — it refuses to split a pair, which is not the same guarantee. Either gate on every pair-carrying chunk:

await Promise.all(
  chunks.flatMap((chunk, i) =>
    chunk.some((e) => e.kind === 'inline-started') ? [commits[i]] : []
  )
);

or add a static assert that 2 * MAX_MAX_INLINE_STEPS <= MAX_BATCH_FANOUT_EVENTS next to the chunker so a constant change fails loudly instead of degrading into the race.

Fixed in 1c64070. Both halves:

• findIndex → a flatMap over every pair-carrying chunk, so the return gates on all of them.
• constants.test.ts pins 2 * MAX_MAX_INLINE_STEPS <= MAX_BATCH_FANOUT_EVENTS with a comment explaining that raising the inline cap has to raise the chunk cap with it (and re-check the server's per-batch transaction budget). Went with a test rather than a type-level assert — the type version needed the literals hardcoded twice, which is worse than what it guards.

AI Review: Note

No changeset. This touches three published packages (@workflow/core, @workflow/world, @workflow/world-vercel) with a user-visible latency/behavior change, so it needs a real pnpm changeset, not --empty.

Separate coverage note while in this file: createBatch is implemented only by world-vercel, so the fold — and every pre-claimed pair path — is unreachable on world-local and world-postgres. No E2E Local * lane exercises this at all; the only end-to-end signal is E2E Vercel Prod / Preview. Worth stating in the changelog page, since it also means the kill switch (WORKFLOW_BATCH_TRANSITIONS=0) is the only lever if this misbehaves in production.

Both done in 1c64070: changeset added (@workflow/core / @workflow/world / @workflow/world-vercel, patch), and this page now states that createBatch is Vercel-World-only so every other World keeps the single-event path and never sends a pair. Also documented the retry rule the other thread changed, since it is now user-visible behavior.

AI Review: Nit

"Mutually exclusive with lazyStepInput" is a comment, not a constraint — StepExecutorParams allows both. runtime.ts upholds it (the ternary sets one or the other), but the executor now has three s.lazyStepInput !== undefined || s.preclaimedStart !== undefined sites plus the params.preclaimedStart === undefined && term in optimisticStart that all quietly depend on it. A discriminated union on the start mode, or an assert(!(params.lazyStepInput && params.preclaimedStart)), would make a future caller's mistake a failure instead of a silently dead branch.

Done in 1c64070 — assert in executeStep rather than a type refactor, since splitting StepExecutorParams into a discriminated union touches every call site for a constraint only one caller can violate.